Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: support user snippets in repositories #784

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat: support user snippets in repositories #784

wants to merge 7 commits into from

Conversation

zhngharry
Copy link
Contributor

@zhngharry zhngharry commented Jun 16, 2024 •
edited
Loading

Based on #435

Implements the functionality that user hosted snippets in own repositories are fetched and shown in marketplace.

Important notes/alternatives considered:

  • I've read through the discussion in Add user created snippets #435, and believe it would be best to have the css code within the manifest, just like we are doing in the official repo right now
  • If this PR is merged, the documentation needs to be adjusted; if a user wants to publicize their snippet from their own repo, they need to add the spicetify-snippets tag to their repo, as well as have a correct manifest entry with the following format:

image

Sidenote: PR #781 fixes the issue that users aren't able to host different types of creations within the same repo

@zhngharry zhngharry requested a review from a team as a code owner June 16, 2024 01:40
@zhngharry zhngharry requested review from theRealPadster and removed request for a team June 16, 2024 01:40
@zhngharry zhngharry changed the title feat: Support User Snippets in Repositories feat: support user snippets in repositories Jun 16, 2024
@rxri
Copy link
Member

rxri commented Jun 16, 2024

We have already discussed it on discord and I already said no to it. This is not something that should be implemented at EOL of v2 basically and it doesn't bring anything good. No one will use it regardless. I'm against of merging this pull request

theRealPadster
theRealPadster approved these changes Jun 22, 2024
View reviewed changes
Copy link
Member

@theRealPadster theRealPadster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a good idea. It would mean people can add their own snippets without needing to submit a PR and have someone from the team take the time to review it. Snippets are very safe compared to extensions, which we allow without manual review.

@theRealPadster
Copy link
Member

What if we make it so it shows the built-in snippets from the repo but also adds support for user snippets? Otherwise we'd lose the ones we have unless we put them somewhere else.

@zhngharry
Copy link
Contributor Author

What if we make it so it shows the built-in snippets from the repo but also adds support for user snippets? Otherwise we'd lose the ones we have unless we put them somewhere else.

Isnt this PR already implementing this, or are you talking abt v3? I don't know if we/the team wants user snippets in v3 because it would kinda disrupt the whole monomanifest flow, no?

To bypass the loss of the current snippets couldn't you just add them to the official modules repo then? Or would that also mean having to filter good/bad snippets for the official repo?

@theRealPadster
Copy link
Member

Ohh, my bad, I didn't read it through fully, I was thinking it just changed snippets to be user-based, which I think is what I had originally done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theRealPadster theRealPadster theRealPadster approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zhngharry @rxri @theRealPadster